Skip to content

Use dnssd for DnsResolver on OSX - #134141

Open
rzikm wants to merge 13 commits into
dotnet:mainfrom
rzikm:dns-resolver-osx
Open

rzikm wants to merge 13 commits into
dotnet:mainfrom
rzikm:dns-resolver-osx

Conversation

@rzikm

@rzikm rzikm commented Sep 17, 2026

Copy link
Copy Markdown
Member

replaces #131934, addresses all feedback on the original PR

Ahmet İbrahim Aksoy and others added 12 commits August 6, 2026 12:05
Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com>
Copilot-Session: f11fb54e-638e-4c9e-ad82-e5d9ff5c20e4
Move DnsSdRecord and the DNSServiceQueryRecord rdata parsers out of
DnsResolverPal.OSX into a new DnsSdRecordParsing static class. Link the
parsing file (and DnsRecords.cs) into System.Net.NameResolution.Unit.Tests
alongside the other production parsers, and add direct unit tests for the
interface-index handling, root name / MX / SRV parsing, TXT framing, and
name-validation edge cases.

The reflection-based tests in DnsResolverTest.cs that reached into private
PAL members are removed in favor of the new unit tests.
…ask.Run

The previous async path wrapped the blocking Poll loop in Task.Run, which
pinned a thread-pool thread for the whole query duration. Wrap the
mDNSResponder fd (returned by DNSServiceRefSockFD) in a non-owning
System.Net.Sockets.Socket via the existing DnsSocket reflection cache and
await Socket.ReceiveAsync(Memory<byte>.Empty, ct) as a real async POLLIN.
DNSServiceProcessResult is still called synchronously when the wait
completes to consume + dispatch the record via the callback.

The sync path keeps Interop.Sys.Poll — a sync caller has already committed
a thread to blocking, and adding async plumbing there would only add
moving parts. Pre-canceled tokens on the async path return
Task.FromCanceled to preserve the TaskCanceledException surface the old
Task.Run(action, ct) shortcut produced.

Addresses feedback from teo-tsirpanis in dotnet#131934.
- Move the embedded-NUL check into DnsResolver.ValidateName so every
  platform rejects NUL-injected names (Windows DnsQueryEx and macOS
  DNSServiceQueryRecord both take null-terminated strings), and drop the
  OSX-only copy.
- Widen DNS labels byte-by-byte in DnsSdRecordParsing.TryParseDnsName
  instead of Encoding.UTF8.GetString, matching how the managed resolver
  decodes response labels (deterministic output for non-UTF-8 bytes).
- Loosen ResolveAddresses_NonExistent_ReturnsNxDomain and the OSX
  CompletesPromptly variant to accept either NoError or NxDomain, since
  mDNSResponder can report NXDOMAIN as NoSuchName or NoSuchRecord
  depending on version.
- Interop.Dnssd now marshals the DNSServiceRef as SafeDnsServiceHandle
  instead of raw IntPtr for DNSServiceQueryRecord/RefSockFD/ProcessResult,
  so handle lifetime is managed by the LibraryImport source generator.
  Move SafeDnsServiceHandle to file scope (internal) with a parameterless
  ctor so the source generator can construct it for out params.
- Collapse the OSX PAL's Query/QueryCore/QueryRecord sync/async pairs
  into single methods that take bool async and branch only at the actual
  wait (Interop.Sys.Poll vs DnsSocket.WaitReadableAsync). Pre-canceled
  check moves up into Query.
- DnsSocket.WaitReadableAsync: switch from Socket.ReceiveAsync with an
  empty buffer to a 1-byte SocketFlags.Peek. An empty-buffer receive
  completes synchronously with zero bytes on Unix (0-byte recv returns
  immediately without ever waiting for POLLIN), so we'd have busy-looped
  calling DNSServiceProcessResult without data. Peek leaves the byte in
  the socket for DNSServiceProcessResult to consume. Dispose the scratch
  Socket via ((IDisposable)socket).Dispose() instead of a reflected
  Dispose delegate.

Verified locally with the OuterLoop async DNS-SD tests (A/AAAA/CNAME
chain, IPv4-only, non-existent, SRV) on macOS arm64 - all 12 pass.
A CNAME query can legitimately return NODATA (NoError with no records)
if the name only has A/AAAA records, so require only that any returned
records are well-formed.

Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Copilot AI lite review requested due to automatic review settings September 17, 2026 15:05
@rzikm
rzikm requested a review from a team September 17, 2026 15:05
@azure-pipelines

Copy link
Copy Markdown
Azure Pipelines:
Successfully started running 3 pipeline(s).
13 pipeline(s) were filtered out due to trigger conditions.
There may be pipelines that require an authorized user to comment /azp run to run.

@dotnet-policy-service

Copy link
Copy Markdown
Contributor

Tagging subscribers to this area: @karelz, @dotnet/ncl
See info in area-owners.md if you want to be subscribed.

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🟡 Changes recommended

Moderate functional-test coverage and CI-regression issues remain unresolved.

Get a fresh assessment by requesting another Copilot review.

Pull request overview

Adds macOS DNS resolution through DNS-SD while retaining the managed resolver for explicit servers.

Changes:

  • Adds DNS-SD interop, PAL logic, socket handling, and record parsing.
  • Adds macOS functional and parser unit-test coverage.
  • Adds embedded-NUL and TXT-record validation.
File summaries
File Summary
src/libraries/System.Net.NameResolution/tests/UnitTests/System.Net.NameResolution.Unit.Tests.csproj Includes DNS-SD parser tests.
src/libraries/System.Net.NameResolution/tests/UnitTests/DnsSdRecordParsingTests.cs Tests DNS-SD record parsing.
src/libraries/System.Net.NameResolution/tests/FunctionalTests/System.Net.NameResolution.Functional.Tests.csproj Configures functional tests for macOS.
src/libraries/System.Net.NameResolution/tests/FunctionalTests/DnsResolverTest.cs Updates resolver functional coverage and platform conditions.
src/libraries/System.Net.NameResolution/tests/FunctionalTests/DnsResolverLoopbackTest.cs Updates loopback resolver coverage.
src/libraries/System.Net.NameResolution/src/System/Net/DnsSocket.cs Refactors socket access and readiness handling.
src/libraries/System.Net.NameResolution/src/System/Net/DnsSdRecordParsing.cs Implements DNS-SD record parsers.
src/libraries/System.Net.NameResolution/src/System/Net/DnsResolverPal.OSX.cs Implements macOS DNS-SD resolution.
src/libraries/System.Net.NameResolution/src/System/Net/DnsResolver.cs Adds embedded-NUL validation.
src/libraries/System.Net.NameResolution/src/System/Net/DnsRecordParsing.cs Exposes TXT parser validity.
src/libraries/System.Net.NameResolution/src/System.Net.NameResolution.csproj Wires macOS sources and interop.
src/libraries/Common/src/Interop/OSX/Interop.Dnssd.cs Adds DNS-SD native bindings.
Review details

Suppressed comments (4)

src/libraries/System.Net.NameResolution/src/System/Net/DnsResolver.cs:523

  • The rationale is inaccurate for the managed resolver: DnsEncodedName.TryEncode builds DNS wire data and rejects the embedded NUL as an invalid label; it does not pass the name to native code as a null-terminated string. Please describe the native and managed validation reasons separately so this comment does not document behavior that the Linux/explicit-server path does not have.
            // Every underlying resolver (Windows DnsQueryEx, macOS DNSServiceQueryRecord,
            // and the managed stub resolver on Linux) passes the name to native code as a
            // null-terminated string, so an embedded NUL would silently truncate the query.

src/libraries/System.Net.NameResolution/tests/FunctionalTests/DnsResolverTest.cs:273

  • Removing the existing Windows Server 2025 guard reintroduces the open CI failure tracked by #131188: this resolver can return ServerFailure for the .invalid name on that queue, but this test still requires NxDomain on Windows. Please restore the guard or relax the assertion to accept the documented resolver-dependent negative result.
        public async Task ResolveAddresses_NonExistent_ReturnsNxDomain(bool async)

src/libraries/System.Net.NameResolution/tests/FunctionalTests/DnsResolverTest.cs:452

  • The removed non-Windows acceptance tests covered the managed PAL contract that nonstandard ports and mixed IPv4/IPv6 server lists are valid. The new OSX overload deliberately delegates explicit-server queries to that PAL, but the remaining loopback tests do not verify mixed-family constructor acceptance; please keep or replace this coverage for OSX as well.
        [ConditionalFact(typeof(PlatformDetection), nameof(PlatformDetection.IsWindows))]

src/libraries/System.Net.NameResolution/tests/FunctionalTests/DnsResolverTest.cs:414

  • This changes the existing static Dns.ResolveAddresses network test to Windows-only, so Linux and macOS no longer exercise the public static API even though Dns.ResolveAddresses remains supported there (Dns.Resolve.cs:17-50) and this PR does not make it Windows-specific. Keep the prior supported-platform condition (or use IsSupportedPlatform) so this coverage is retained on Unix and macOS.
        [ConditionalTheory(typeof(PlatformDetection), nameof(PlatformDetection.IsWindows))]
  • Files reviewed: 12/12 changed files
  • Comments generated: 3
  • Review effort level: Lite

Comment thread src/libraries/System.Net.NameResolution/src/System.Net.NameResolution.csproj Outdated
Comment thread src/libraries/System.Net.NameResolution/tests/FunctionalTests/DnsResolverTest.cs Outdated
Comment thread src/libraries/System.Net.NameResolution/src/System/Net/DnsSocket.cs Outdated
Comment thread src/libraries/System.Net.NameResolution/src/System/Net/DnsSocket.cs Outdated
- Restore DnsResolver_UnsupportedPlatform_ThrowsPlatformNotSupported
  coverage for the Browser/WASI DnsResolverPal.Unsupported query path,
  updated to use the instance DnsResolver API.
- Remove the now-unused Poll interop includes from
  System.Net.NameResolution.csproj; the OSX async path waits via
  DnsSocket.ReceiveAsync instead of polling.
- Rename the misleading "Windows network tests" comment heading to
  reflect that these tests run on all supported platforms.
- Simplify DnsSocket.Dispose to cast to IDisposable directly and drop
  the redundant UnsafeAccessor Dispose wrapper.

Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Copilot AI review requested due to automatic review settings September 18, 2026 10:13

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🔵 Needs a closer look

Address the two unresolved moderate correctness findings before approval.

Review details

Suppressed comments (2)

Previously missed (2) — in code that hasn't changed since the last review.

src/libraries/System.Net.NameResolution/src/System/Net/DnsResolverPal.OSX.cs:270

  • Because this query requests kDNSServiceFlagsReturnIntermediates, mDNSResponder can invoke the callback for a CNAME/intermediate record whose rrtype is not _requestedType. Returning before processing kDNSServiceFlagsMoreComing leaves IsComplete false when that callback is the final callback, so DNSServiceProcessResult is called again and can block until timeout for CNAME-only/NODATA answers. Process the completion flag independently of whether the record is one of the requested types.
    src/libraries/System.Net.NameResolution/src/System/Net/DnsSdRecordParsing.cs:34
  • TryParseAddress validates only the RDATA length, not that the length matches record.Type. A malformed A record with 16 bytes is therefore returned as an IPv6 AddressRecord, and a malformed AAAA record with 4 bytes is returned as IPv4. The managed parser rejects both cases (TryParseARecord/TryParseAAAARecord), so preserve that contract here by requiring A+4 or AAAA+16 before constructing the address (and add the corresponding parser tests).
  • Files reviewed: 12/12 changed files
  • Comments generated: 0 new
  • Review effort level: Lite

This branch has not been deployed

No deployments
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants